Add database provider abstraction layer#41
Conversation
Introduce a DatabaseProvider interface that abstracts database-specific operations (replication status, read-only control, start/stop replication) behind a common contract. This is the architectural foundation for multi-database support. - Define DatabaseProvider interface and ReplicationStatus type (provider.go) - Implement MySQLProvider that delegates to existing DAO functions (provider_mysql.go) - Add provider registry with MySQL as default provider (provider_registry.go) - Add unit tests for interface satisfaction and registry (provider_test.go) - Add documentation for the provider system (docs/database-providers.md) Closes #32
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational database provider abstraction layer within Orchestrator. By introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed database provider abstraction layer, decoupling core logic from database-specific operations. The changes include a new DatabaseProvider interface, a default MySQLProvider implementation, a thread-safe registry, and comprehensive documentation. The code is clean and the approach is solid. I have one suggestion to improve maintainability by reducing code duplication in the MySQLProvider implementation.
| func (p *MySQLProvider) IsReplicaRunning(key InstanceKey) (bool, error) { | ||
| instance, err := ReadTopologyInstance(&key) | ||
| if err != nil { | ||
| return false, log.Errore(err) | ||
| } | ||
| return instance.ReplicaRunning(), nil | ||
| } |
There was a problem hiding this comment.
The IsReplicaRunning method duplicates the logic for fetching the instance topology from GetReplicationStatus. To improve maintainability and reduce code duplication, consider implementing IsReplicaRunning by calling GetReplicationStatus and returning the ReplicaRunning field from the resulting status object. This makes it clear that IsReplicaRunning is a convenience helper for GetReplicationStatus.
| func (p *MySQLProvider) IsReplicaRunning(key InstanceKey) (bool, error) { | |
| instance, err := ReadTopologyInstance(&key) | |
| if err != nil { | |
| return false, log.Errore(err) | |
| } | |
| return instance.ReplicaRunning(), nil | |
| } | |
| func (p *MySQLProvider) IsReplicaRunning(key InstanceKey) (bool, error) { | |
| status, err := p.GetReplicationStatus(key) | |
| if err != nil { | |
| return false, err | |
| } | |
| return status.ReplicaRunning, nil | |
| } |
There was a problem hiding this comment.
Pull request overview
Introduces an initial database-provider abstraction in go/inst to decouple engine-specific operations (starting with MySQL) behind a common DatabaseProvider interface, enabling future multi-database support.
Changes:
- Added
DatabaseProviderinterface and a database-agnosticReplicationStatusmodel. - Implemented
MySQLProviderand set it as the default via a thread-safe global provider accessor. - Added unit tests for provider wiring and documentation describing how to add new providers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go/inst/provider.go | Defines DatabaseProvider interface and ReplicationStatus model. |
| go/inst/provider_mysql.go | Implements the MySQL provider by delegating to existing instance/DAO operations. |
| go/inst/provider_registry.go | Adds global provider storage with RWMutex and MySQL default initialization. |
| go/inst/provider_test.go | Adds tests for default provider and basic provider behaviors. |
| docs/database-providers.md | Documents the provider abstraction and how to implement/register providers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // manages. The default provider is MySQL. | ||
| func SetProvider(p DatabaseProvider) { | ||
| providerMu.Lock() | ||
| defer providerMu.Unlock() |
There was a problem hiding this comment.
SetProvider currently allows setting a nil provider, which would make GetProvider() return nil and can lead to panics at call sites (e.g., GetProvider().ProviderName()). Consider rejecting nil (return an error or panic with a clear message) or keeping the existing provider when p is nil, so the global provider is always non-nil after init.
| defer providerMu.Unlock() | |
| defer providerMu.Unlock() | |
| if p == nil { | |
| // Ignore attempts to set a nil provider to avoid panics from GetProvider(). | |
| // The existing provider (including the default set in init) is preserved. | |
| return | |
| } |
|
|
||
| position := instance.ExecutedGtidSet | ||
| if position == "" { | ||
| position = instance.SelfBinlogCoordinates.DisplayString() |
There was a problem hiding this comment.
If the instance has neither ExecutedGtidSet nor a valid binlog coordinate (SelfBinlogCoordinates.LogFile is empty), DisplayString() will produce the sentinel ":0", which looks like a real position but is not meaningful. Consider returning an empty Position (or another explicit "unknown" sentinel) when coordinates are empty, to avoid consumers misinterpreting ":0" as a valid replication position.
| position = instance.SelfBinlogCoordinates.DisplayString() | |
| // Only derive a position from binlog coordinates if they are valid. | |
| // When LogFile is empty, DisplayString() may return a sentinel like ":0", | |
| // which looks like a real position but is not meaningful. In that case, | |
| // leave position empty to clearly indicate that it is unknown. | |
| if instance.SelfBinlogCoordinates.LogFile != "" { | |
| position = instance.SelfBinlogCoordinates.DisplayString() | |
| } |
| instance, err := ReadTopologyInstance(&key) | ||
| if err != nil { | ||
| return false, log.Errore(err) | ||
| } | ||
| return instance.ReplicaRunning(), nil |
There was a problem hiding this comment.
IsReplicaRunning duplicates the same ReadTopologyInstance call already done by GetReplicationStatus. To reduce duplication and keep semantics consistent if the logic changes, consider implementing IsReplicaRunning by calling GetReplicationStatus and returning status.ReplicaRunning.
| instance, err := ReadTopologyInstance(&key) | |
| if err != nil { | |
| return false, log.Errore(err) | |
| } | |
| return instance.ReplicaRunning(), nil | |
| status, err := p.GetReplicationStatus(key) | |
| if err != nil { | |
| return false, err | |
| } | |
| return status.ReplicaRunning, nil |
| if status.Lag != 0 { | ||
| t.Error("expected Lag to be 0 by default") | ||
| } |
There was a problem hiding this comment.
This test asserts Go zero-value behavior (Lag == 0) but the documented contract for ReplicationStatus says "Lag of -1 means unknown" (also repeated in docs/database-providers.md). This makes the test misleading and likely to become a blocker if you later introduce a constructor/default that sets Lag to -1. Consider removing this test, or changing it to validate the documented semantics instead of zero-values.
| if status.Lag != 0 { | |
| t.Error("expected Lag to be 0 by default") | |
| } |
Summary
DatabaseProviderinterface that abstracts database-specific operations (replication status discovery, read-only control, replication start/stop) behind a common contractMySQLProvideras the default provider, delegating to existing DAO functions with zero behavior changeThis is the architectural foundation for multi-database support — MySQL becomes the first provider, and future engines (e.g., PostgreSQL) can be added by implementing the same interface.
Closes #32
Test plan
go buildsucceeds with no new errorsTestMySQLProviderName,TestDefaultProviderIsMySQL,TestMySQLProviderImplementsInterface,TestSetAndGetProvider,TestReplicationStatusDefaults)MySQLProviderimplementsDatabaseProvidergo/insttests